-
Notifications
You must be signed in to change notification settings - Fork 675
feat: editors #4102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: editors #4102
Conversation
|
You the goat of reviewing :D Cheers |
archinstall/applications/editor.py
Outdated
| def _get_editor_binary(self, editor: Editor) -> str: | ||
| # special handling only name that differs | ||
| return 'nvim' if editor == Editor.NEOVIM else editor.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The paradigm used by this repo is to add functionality like this to the enum. Consider following that convention and moving this method to Editor instead and removing the underscore(_) that designates it as intending to be private.
Here are two instances of this paradigm being used in the repo (there are more):
FilesystemType.installation_pkg
archinstall/archinstall/lib/models/device.py
Lines 814 to 824 in 5fbe1de
@property def installation_pkg(self) -> str | None: match self: case FilesystemType.Btrfs: return 'btrfs-progs' case FilesystemType.Xfs: return 'xfsprogs' case FilesystemType.F2fs: return 'f2fs-tools' case _: return None GfxDriver.gfx_packages
archinstall/archinstall/lib/hardware.py
Line 83 in 5fbe1de
def gfx_packages(self) -> list[GfxPackage]:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this suggestion can be ignored because you are following the convention set by archinstall/lib/models/application.py and other files in archinstall/applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See latest commit ;)
|
I'm quite worried at this point to go down a rabbit hole with this new application configuration. If we decide to give this specific configuration option in the menu there would be an argument to simply provide dozens of options for users to configure. This is user software to be installed not a system configuration to get Arch up and running. It is not necessary for a working OS at all. The reason an editor was included in the profiles is to provide at least one out of the box so it can be used to edit files, ideally that should be the default a profile ships with otherwise. |
This patch removes
vimandnanofrom default indesktopand gives a menu option forEditorinside ofApplications. This also properly enables it to be used as default through/etc/environmentFixes the preview in main menu too and makes it more in line so that if you perform a minimal install you might still want to be able to edit files. This also enables
$EDITORglobal key value pair to be used consistentlyGuys help I'm stuck in VIM